[ContributorDoc] Checklist for adding new LSP/BSP messages#2626
Merged
Conversation
Member
Author
|
@swift-ci Please test |
rintaro
commented
Apr 28, 2026
|
|
||
| - [ ] Add the request Swift file under `Sources/LanguageServerProtocol/Requests/` | ||
| - [ ] `Sources/LanguageServerProtocol/CMakeLists.txt` — add the file in alphabetical order | ||
| - [ ] `Sources/LanguageServerProtocol/Messages.swift` — add to `builtinRequests` |
Member
Author
There was a problem hiding this comment.
This tend to be missed because our test cases don't go through this. We should fix that...
rintaro
commented
Apr 28, 2026
ahoppen
reviewed
Apr 28, 2026
| - [ ] `Sources/SourceKitLSP/MessageHandlingDependencyTracker.swift` — add a case to `init(_ request:)` to declare the dependency type (`.freestanding`, `.documentRequest(uri)`, etc.) *(client → server only)* | ||
| - [ ] `Sources/SourceKitLSP/SourceKitLSPServer.swift`: | ||
| - [ ] *(client → server)* Add a case to the main request dispatch switch and implement the handler | ||
| - [ ] *(server → client)* Call `client.send(request)` from the appropriate place |
Member
There was a problem hiding this comment.
I’m not sure if we need this. I mean, if you want to use it, you have to send the request, otherwise there’s just no point.
| - [ ] `Contributor Documentation/LSP Extensions.md` — document the request | ||
| - [ ] Add tests | ||
|
|
||
| ## LSP Notification |
Member
There was a problem hiding this comment.
Should we unify this with the LSP Requests section? Most of it is duplicated.
| - [ ] `Sources/SourceKitLSP/SourceKitLSPServer.swift`: | ||
| - [ ] *(client → server)* Add a case to the main request dispatch switch and implement the handler | ||
| - [ ] *(server → client)* Call `client.send(request)` from the appropriate place | ||
| - [ ] If experimental: advertise via `serverCapabilities()` and guard with a capability check *(client → server)*; check the client capability before sending *(server → client)* |
Member
There was a problem hiding this comment.
We need to advertise the request from both SourceKitLSPServer.serverCapabilities and SwiftLangaugeService.initialize.
| - [ ] If experimental: advertise via `serverCapabilities()` and guard with a capability check *(client → server)*; check the client capability before sending *(server → client)* | ||
| - [ ] `Sources/SourceKitLSP/CapabilityRegistry.swift` — if gated on an experimental **client** capability, add a helper property *(client → server only)* | ||
| - [ ] `Contributor Documentation/LSP Extensions.md` — document the request | ||
| - [ ] Add tests |
Member
There was a problem hiding this comment.
I don’t think we should explicitly need to mention this. Tests should be added to every change, no matter what.
| - [ ] `Sources/BuildServerIntegration/BuildServerMessageDependencyTracker.swift` — add a case to the dependency tracking switch | ||
| - [ ] `Sources/BuildServerIntegration/BuiltInBuildServer.swift` — add the handler method to the `BuiltInBuildServer` protocol | ||
| - [ ] `Sources/BuildServerIntegration/BuiltInBuildServerAdapter.swift` — add a case to the request dispatch switch | ||
| - [ ] Implement the handler in all `BuiltInBuildServer` conformers: |
Member
There was a problem hiding this comment.
I don’t think we need to explicitly spell this out. Adding the requirement to BuiltInBuildServer will prompt theses.
rintaro
force-pushed
the
add-message-doc
branch
2 times, most recently
from
53769e4 to
a2da4b9
Compare
ahoppen
approved these changes
Apr 28, 2026
Member
ahoppen
left a comment
ahoppen
left a comment
There was a problem hiding this comment.
Some more nitpicks, otherwise looks good.
| - [ ] If handled by a language service: also advertise in each `LanguageService.initialize` conforming method | ||
| - [ ] If experimental *(server → client)*: check the client capability before sending | ||
| - [ ] `Sources/SourceKitLSP/CapabilityRegistry.swift` — if gated on an experimental **client** capability, add a helper property | ||
| - [ ] `Contributor Documentation/LSP Extensions.md` — document if it is a SourceKit-LSP extension |
Member
There was a problem hiding this comment.
Suggested change
| - [ ] `Contributor Documentation/LSP Extensions.md` — document if it is a SourceKit-LSP extension | |
| - [ ] `Contributor Documentation/LSP Extensions.md` — document if it is an LSP extension |
| - [ ] `Sources/BuildServerIntegration/BuildServerMessageDependencyTracker.swift` — add a case to the dependency tracking switch *(request only)* | ||
| - [ ] `Sources/BuildServerIntegration/BuiltInBuildServer.swift` — add the handler method to the `BuiltInBuildServer` protocol *(request only)* | ||
| - [ ] `Sources/BuildServerIntegration/BuiltInBuildServerAdapter.swift` — add a case to the request dispatch switch *(request only)* | ||
| - [ ] *(SourceKit-LSP → build server, notification only)* Send via `buildServerManager.send(notification)` from the appropriate place; if built-in servers also need to react, add a case to `BuiltInBuildServerAdapter.handle(notification:)` |
Member
There was a problem hiding this comment.
Similar to my comment in LSP: I don’t think we need to document that you need to send the request if you want to send it.
| - [ ] `Sources/BuildServerIntegration/BuiltInBuildServerAdapter.swift` — add a case to the request dispatch switch *(request only)* | ||
| - [ ] *(SourceKit-LSP → build server, notification only)* Send via `buildServerManager.send(notification)` from the appropriate place; if built-in servers also need to react, add a case to `BuiltInBuildServerAdapter.handle(notification:)` | ||
| - [ ] *(build server → SourceKit-LSP, notification only)* `Sources/BuildServerIntegration/BuildServerManager.swift` — add a case to `handle(notification:)` and implement the handler | ||
| - [ ] `Contributor Documentation/BSP Extensions.md` — document if it is a SourceKit-LSP extension |
Member
There was a problem hiding this comment.
Suggested change
| - [ ] `Contributor Documentation/BSP Extensions.md` — document if it is a SourceKit-LSP extension | |
| - [ ] `Contributor Documentation/BSP Extensions.md` — document if it is a BSP extension |
rintaro
force-pushed
the
add-message-doc
branch
from
a2da4b9 to
6598efb
Compare
rintaro
force-pushed
the
add-message-doc
branch
from
6598efb to
41f2146
Compare
Member
Author
|
@swift-ci Please test |
Member
Author
|
@swift-ci Please test Windows |
ahoppen
approved these changes
Apr 29, 2026
|
Thank you for adding this doc, Rintaro! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Because I keep forgetting these.